-
Notifications
You must be signed in to change notification settings - Fork 451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update PowerShellRunner to support hard link creation and enable more functional tests on Mac #266
Update PowerShellRunner to support hard link creation and enable more functional tests on Mac #266
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor comments, feel free to take them or not.
{ | ||
return new[] | ||
FileSystemRunner fileSystem = runner.ToList().First() as FileSystemRunner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pick: you're using as
but not checking for null. I would just do a normal cast here so we end up with a more appropriate exception if someone ever inserts in invalid type into the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just do a normal cast here so we end up with a more appropriate exception if someone ever inserts in invalid type into the list.
Sounds good I'll fix this up
return hardLinkRunners.ToArray(); | ||
} | ||
|
||
// Always return at least one runner that supports creating hard links |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it a bug in our code if we get this far? I would just throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanoursa this is not a bug because when we run without --full-suite
the only runner will be SystemIORunner
which does not support hard link creation.
I can either update the comment to include those details, or I can mark the test as full suite only, let me know if you have a preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, yea a comment would be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I just noticed that we had this same issue for RenameDirectory
and addressed it by having SystemIORunner.RenameDirectory
PInvoke to the native API. I'll see if I can do the same for hard link creation and then all of this "does runner support hard link" code can be removed.
a3ce35e
to
4c38ca0
Compare
d160760
to
bc6f896
Compare
Fixes #262
CreateHardLink
implementation toPowerShellRunner
ModifiedPathsTests
when building list of runnersCheckoutTests.CheckoutBranchThatHasFolderShouldGetDeleted
test on MacGitCommands.RebaseTests
on Mac